Skip to content

Conversation

ada4a
Copy link
Contributor

@ada4a ada4a commented Sep 24, 2025

Fixes #15307

changelog: [map_entry]: provide explicit type when suggesting None

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 24, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 24, 2025

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

peel_hir_expr_while,
};
use core::fmt::{self, Write};
use core::fmt::Write;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, why not use std::fmt::Write since std::fmt is also imported?

"e.insert({})",
snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0
);
let value_str = snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're cleaning up, "_" would be a better choice than ".." for a single value (even though this is never used…). Same below.

));
res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0);
let value_str = snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0;
let _: fmt::Result = write!(res, "{value_str}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not res.push_str(&value_str); which is infallible?

fn issue15307<K: Eq + Hash, V>(mut m: HashMap<K, V>, k: K, v: V) {
if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
//~^ map_entry
assert!({ e.insert(v); None::<V> }.is_none());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so ugly.

If the user is using the result of m.insert(…) which we know to be None (as opposed to dropping it), can't we just suggest the same thing as we currently do, but add that m.insert(…) would always have returned None, and suggest to rewrite the code to not depend on this value or to add an explicit type to None if needed?

I dislike suggesting a type from a rustc_middle::ty::Ty when it will not be what the user would write when aliases are involved for example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More generally, I don't like us generating fixes that we know to be of a very bad style just to make it compatible with the surrounding code. In this case, we might as well warn about the performance issue, but not suggest an autofix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementing this required some substantial changes, so I opened a v2 of the PR: #15808

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 29, 2025
@ada4a
Copy link
Contributor Author

ada4a commented Oct 3, 2025

Closed in favor of #15808

@ada4a ada4a closed this Oct 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure to correctly apply fixes for clippy::map_entry
3 participants